Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added Text Alignments. and fixes #583. Changed Clicked from Action to EventHandler per tenets. #621

Closed
wants to merge 3 commits into from

Conversation

BDisp
Copy link
Collaborator

@BDisp BDisp commented Jun 5, 2020

No description provided.

@tig
Copy link
Collaborator

tig commented Jun 5, 2020

This is really good work.

I don't think we can accept this until we have a clear conversation with @migueldeicaza on backwards compat. Most of these changes from Action to EventHandler will break existing apps.

If you look at the inventory I created in #447, I suggested how we could do this in a way that is backwards compatible.

To be clear, my opinion is that we do what you have done and "do it right" and not clutter the code with a ton of legacy APIs. But Miguel knows more about what he's promised existing users and I think it is his call on whether we do this or not.

Because of this, it might be a good idea to break your TextAlignment changes out as a separate PR because that stuff looks like a no brainer.

@migueldeicaza
Copy link
Collaborator

I have never really liked EventHandlers, going all the way back to WinForms, and would rather use Actions which were introduced later.

Is there a reason to embrace that idiom, other than "This is what .NET in 2001 did"?

@tig
Copy link
Collaborator

tig commented Jun 5, 2020

I have never really liked EventHandlers, going all the way back to WinForms, and would rather use Actions which were introduced later.

Is there a reason to embrace that idiom, other than "This is what .NET in 2001 did"?

Yes, I think I and others pretty clearly made the case for them here:

#101

Action was not invented to replace EventHandler but to compliment it. Action lacks pub/sub multiplexing, which is really important for UI idioms where multiple pieces of code want to subscribe to an event. We could build a pub/sub model on top of Action, but then....

@migueldeicaza please read and comment on this: #447

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 5, 2020

It really gets a little difficult to work like that. If it is to impose big changes, the sooner the better, that is, before version 1.0 and not after getting used to the new model where you will see that there are different treatments for the same events. @migueldeicaza really @tig is right to adopt Event / EventHandler for the user's UI, because it allows for an interaction between the API and UI so that the user receives information about the object being passed (sender) and the information needed to interact with the API (e) that allows you to both read and send content. For example, in the case of Button why create a Selected event if it only serves to press or click and have two events to do the same thing?. User level changes are not very complicated, it is practically putting (o, e) inside the parentheses. I would like these rules to be well defined so that we do not have the feeling of wasting time. Thanks and sorry for something.

@BDisp BDisp force-pushed the button-text-alignments branch from 9c8bb5b to 431c37a Compare June 5, 2020 09:29
@migueldeicaza
Copy link
Collaborator

One thing that many folks do not realize is that every .NET delegate is multicast, which means that this works and does not even need "Action" to be flagged as an event:

Action a;

void setup ()
{
    a += method1;
    a += method2;
}

And this calls both methods.

The scenario that @BDisp mentions is the demultiplexing of code, which is a rarely used feature, and everyone has to pay the price for it. A solution that made sense when we did not have lambdas. Nowadays the same demultiplexing can take place inline:

void SharedCode (int button)
{
   switch (button) {...}
}


button1.action = () => { SharedCode (1); }
button2.action = () => { SharedCode (2); }

Now, these are all old-way of managing events. A more interesting discussion is whether embracing System.Reactive is the right approach, as it is a better framework for reasoning about events than these hooks.

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 5, 2020

We will have to pay a price for that, but with the increasingly powerful machines I think it would be insignificant. For me you are the master and your opinion counts a lot and what you decide I will always follow :-)

@tig
Copy link
Collaborator

tig commented Jun 5, 2020

Ok, I was obviously wrong on Action and need to revisit some of my deepest thoughts. I will ponder this and consider what to do. I do think, in the end, being consistent is super important for a framework like this.

@BDisp thanks for your work on this PR, but we are not going to merge it at this time.

@tig tig closed this Jun 5, 2020
@BDisp
Copy link
Collaborator Author

BDisp commented Jun 5, 2020

Certainly @tig I understand. I will then submit the TextAlignment only. Thanks.

@tig
Copy link
Collaborator

tig commented Jun 5, 2020

Certainly @tig I understand. I will then submit the TextAlignment only. Thanks.

It would be awesome if you took a swing at re-factoring the keyboard events using the draft guidelines I just put here: https://github.com/migueldeicaza/gui.cs/wiki/Framework-Design-Guidelines

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 5, 2020

@tig I already have three Views ready to submit PR and they all use EventHandler with custom EventArgs. Do I have to change everything back to Action before I submit PR?
These are new changes and do not change anything that already exists.

@tig
Copy link
Collaborator

tig commented Jun 5, 2020

I think so.

This is all my fault because I pushed forward without waiting to hear from @migueldeicaza. Sorry about that.

Let's look at it as an opportunity to do it right and do it consistently though! ;-)

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 5, 2020

It would be awesome if you took a swing at re-factoring the keyboard events using the draft guidelines I just put here: https://github.com/migueldeicaza/gui.cs/wiki/Framework-Design-Guidelines

@tig do you mean these?

public override bool ProcessKey (KeyEvent keyEvent)

public override bool ProcessHotKey (KeyEvent keyEvent)

public override bool ProcessColdKey (KeyEvent keyEvent)

@tig
Copy link
Collaborator

tig commented Jun 5, 2020

Well, those are the event raising functions, but, yes, those are the events I was asking about.

To be clear on what I'm asking:

  • I believe we should be consistent on how events are defined.
  • I believe we should do this before 1.0
  • I now believe we should rally around Action<T>.

I was hoping you'd take a look at those events and see how hard it would be to convert them to be Action<T> based. How hard would it be for subscribers to adapt?

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 5, 2020

I don't think it's that hard. I will contribute the best I can.

@grahamehorner
Copy link

I'm a +1 for @migueldeicaza #reactive suggestion especially given the power of linq over events :D

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 6, 2020

@tig changing from EventHandler to Action is relatively easy. The question I have is how to proceed in those cases where the sender object is actually used to use its properties without having to declare the View's that are using it as public. I'm already dealing with that in the Terminal.Gui project.

@tig
Copy link
Collaborator

tig commented Jun 6, 2020

@tig changing from EventHandler to Action is relatively easy. The question I have is how to proceed in those cases where the sender object is actually used to use its properties without having to declare the View's that are using it as public. I'm already dealing with that in the Terminal.Gui project.

In the cases where sender is used, WHY is it used? Is the subscriber not able to access the original object?

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 6, 2020

I found this into Designer project for a generic handler. Really in that case the subscriber should be able to provide his original object. I'll ignore it.

		private static void LoginText_Leave (object sender, EventArgs e)
		{
			((TextField)sender).Text = $"Leaving from: {sender}";
		}

		private static void LoginText_Enter (object sender, EventArgs e)
		{
			((TextField)sender).Text = $"Entering in: {sender}";
		}

		private static void LoginText_MouseLeave (object sender, View.MouseEventEventArgs e)
		{
			((TextField)sender).Text = $"Mouse leave at X: {e.MouseEvent.X}; Y: {e.MouseEvent.Y} HasFocus: {e.MouseEvent.View.HasFocus}";
		}

		private static void LoginText_MouseEnter (object sender, View.MouseEventEventArgs e)
		{
			((TextField)sender).Text = $"Mouse enter at X: {e.MouseEvent.X}; Y: {e.MouseEvent.Y} HasFocus: {e.MouseEvent.View.HasFocus}";
		}

@BDisp
Copy link
Collaborator Author

BDisp commented Jun 6, 2020

@tig it's all done in PR #632

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants